[68697] Deselecting tree view items in single select does not work - #378
Conversation
🦋 Changeset detectedLatest commit: 7f3e4ef The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull Request Overview
This PR enables deselecting items in single-select tree views by clicking on the currently selected item. Previously, single-select tree views did not allow unchecking the checked item.
Key changes:
- Modified
handleSingleSelectionmethod to support toggling selection state - Removed logic that prevented unchecking in single-select mode
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/components/primer/alpha/tree_view/tree_view.ts | Updated handleSingleSelection to allow deselecting the currently selected item when clicked again |
| .changeset/sour-news-sip.md | Added changeset documenting the new behavior allowing deselection in single-select mode |
Comments suppressed due to low confidence (2)
app/components/primer/alpha/tree_view/tree_view.ts:337
- The
activeNodesgetter usesdocument.querySelectorAllwhich queries the entire document rather than being scoped to this tree view component. When unchecking nodes in line 215, this will incorrectly uncheck all checked nodes across all tree views on the page, not just the current one. Change tothis.querySelectorAll('[aria-checked=\"true\"]')to scope it to the current component.
get activeNodes() {
return document.querySelectorAll('[aria-checked="true"]')
}
app/components/primer/alpha/tree_view/tree_view.ts:226
- The
treeViewNodeCheckedevent uses stalenodeInfothat was created at line 196 before the checked state was updated. This means the event will report incorrectcheckedValueandpreviousCheckedValue. ThenodeInfoshould be regenerated after the checked state changes, or the event should be moved into the if/else branches with correctly computed values.
this.dispatchEvent(
new CustomEvent('treeViewNodeChecked', {
bubbles: true,
detail: [nodeInfo],
}),
)
9abda80 to
7f3e4ef
Compare
|
@HDinger due to time constraints we skipped adding a test for this behaviour change. However, this is something we should still try to do (particularly before upstreaming) |
| // disallow unchecking checked item in single-select mode | ||
| if (!currentlyChecked) { | ||
| if (this.getNodeCheckedValue(node) === 'true') { | ||
| this.setNodeCheckedValue(node, 'false') |
There was a problem hiding this comment.
I'd have preferred this.uncheckAtPath(path)
What are you trying to accomplish?
Allow deselecting the selected item, while clicking on it.
Closes https://community.openproject.org/wp/68697
Risk Assessment